-
-
Notifications
You must be signed in to change notification settings - Fork 14
Generate docs on install, nicer URLs and some caching #107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
- footer sticks to the bottom of the window - improve module info display - info icon size and alignment - module info boottom and right margin - module description foont size - improve function info display - remove duplicate function name
- rewrite scan.xql to generate.xqm - adhere to naming conventions - explicitly allow routes and deny evertying else - route modules/reindex.xql moved to /regenerate - add routes fro markdown and static resources - call to /regenerate is a GET request
- use new route `/exist/apps/fundocs/generate` - do not test for removed headline in function info - test for changed module name
- generate documentation in the finish step of the package installation - add cache headers to most routes that could benefit from client-side caching - the html part is dropped from all URLs - /index.html -> / - /view.html -> /view - /ajax.html -> /query - /browse.html -> /browse - reliably set the URL on all rendered HTML pages - move all page templates to /templates/pages
Will the old URLs be maintained? Many people will have these bookmarked and they are likely also in printed documents including possibly the eXist-db Book. As the W3C say - "Cool URIs don't change" - https://www.w3.org/Provider/Style/URI |
- add arity - add annotations - add return with occurrence (return in comment left intact) - add parameters with occurrence (param elements in comment left intact)
The breaking nature is the reason I decided to open an extra PR. User facing URLs that could have been bookmarked or printed:
|
There are a LOT more URLs than that @line-o |
Would you be willing to share an example URL I missed? |
or we "chose them very badly" :-) anyway @adamretter it really helps to provide a few concrete samples in addition to the oneliner. |
I just re-checked the controller and there is exactly one route matching |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
+1 for major version bump.
@line-o I think we could add custom forwards for the 4 old URLs ending in .html
- redirect /index.html to / with parameters q, action and map type parameter to where - redirect /browse.html to /browse with parameters w3c, extensions and appmodules - redirect /view.html to /view with parameters location, uri and function - add tests for redirects
The function documentation is now built when installing the package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great stuff, thanks @line-o . The tests are hardcoding the host, which will break when running with a different baseUrls. pathname
takes globs and can be used instead of url
to maintain test isolation. see cy.location
|
||
context('Visiting a legacy URL', () => { | ||
it('redirects to the new index route', () => { | ||
cy.visit('index.html?action=search&type=name&q=tei') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would us qs
object here and in the subsequent visits, for better legibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@duncdrum could you be more specific please. I am not familiar with all things Cypress.
context('Visiting a legacy URL', () => { | ||
it('redirects to the new index route', () => { | ||
cy.visit('index.html?action=search&type=name&q=tei') | ||
cy.url().should('equal', 'http://localhost:8080/exist/apps/fundocs/?action=search&where=everywhere&q=tei') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cy.location('pathname')
will let you test the redirects without needing to hardcode the host
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test also asserts the right querystring. Is that what qs
is for from the earlier comment?
it('succeeds', () => { | ||
cy.timeout(10000); // generation needs some time | ||
cy.request({ | ||
url: 'http://127.0.0.1:8080/exist/apps/fundocs/regenerate', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use baseUrl
fixed in the earlier PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a pity that we have two divergent PRs now.
@line-o No, I did not confuse them. Your change breaks all existing URI's that were book-markable and/or printable. These URIs have been stable for probably over a decade and appear in many places outside of the eXist-db project - including in print. Three (of many many possible) examples: There is no value in breaking these stable URIs! Please undo these changes. |
I checked the URLs as mentioned, all work perfectly well, backwards compatible. |
@adamretter after building the current content of this PR and deploying locally all the mentioned URLS (host part |
@reinhapa @dizzzz Sorry I don't understand, in the description of the PR by @line-o he writes:
Above @line-o himself acknowledges that this is a breaking change! Breaking such URLs after many many years is a very very bad idea! |
Please avoid using "!" so frequently in your comments; I find these 'not so nice'. The proof is in the pudding, 1st we did a code inspection and then we performed the actual testing where literally all URLs turned out to be functional and backwards compatible. |
This PR is built on top of #106
The client-facing URLs are changing, legacy URLs are redirected to their new counterpart
- /index.html -> /
- /view.html -> /view
- /ajax.html -> /query
- /browse.html -> /browse
the
type
parameter for queries was renamed towhere
to better indicate what the parameter does. It determines where the entered term is searched in; "everywhere" being the default.Other enhancements